Skip to content

Simplification of AMSMessage memory management#110

Merged
koparasy merged 6 commits intodevelopfrom
feature/memory-management-rmq
Mar 21, 2025
Merged

Simplification of AMSMessage memory management#110
koparasy merged 6 commits intodevelopfrom
feature/memory-management-rmq

Conversation

@lpottier
Copy link
Member

This PRs simplify the memory management of AMSMessage by keeping track of what messages have not been acknowledged or never delivered. These messages are being re-send as soon as possible.

The bookkeeping is done by AMSMessageRecords, a fine wrapper around a std::unordered_map allowing safe concurrent accesses.

Each message is recorded as a memory blob (shared pointer) and a size.

  • RMQInterface create a shared_ptr of a AMSMessageRecords and send a copy to RMQPublisherHandler
  • RMQPublisherHandler will then update AMSMessageRecords
  • In case of failure of the broker, RMQPublisherHandler is destroyed.
    • Then, RMQInterface creates a new RMQPublisherHandler with a new copy of the shared_ptr owning the AMSMessageRecords.

…n correctly acked are stored into a hashmap to be resend later.

Signed-off-by: Loic Pottier <pottier1@llnl.gov>
Signed-off-by: Loic Pottier <pottier1@llnl.gov>
@lpottier lpottier requested a review from koparasy March 21, 2025 05:11
@lpottier
Copy link
Member Author

I am not fully satisfied here, especially because AMSMessage currently introduces a memory leak if it not used properly. In our case, because of the shared_ptr, the memory is freed. But if you use AMSMessage to create a temporary message, for example:

void foo() {
  AMSMessage msg (...);
  // code here...
} // msg._data is not freed here

But if you create a shared_ptr you are good:

void foo() {
  AMSMessage msg (...);
  std::shared_ptr<uint8_t> ptr(msg.data());
} // msg._data is freed here

However, it is counterintuitive and not RAII. Ideally, msg._data would be smart pointer from the beginning.

@koparasy
Copy link
Member

@lpottier I further extended your logic. We now have a persistent map of unAck messages. This is implemented as a singleton. All threads can be aware at any time about these messages. It removes some of our logic in passing the AMSMessageRecords around.

I also fixed the data race I pointed out earlier.

@koparasy
Copy link
Member

I am not fully satisfied here, especially because AMSMessage currently introduces a memory leak if it not used properly. In our case, because of the shared_ptr, the memory is freed. But if you use AMSMessage to create a temporary message, for example:

void foo() {
  AMSMessage msg (...);
  // code here...
} // msg._data is not freed here

But if you create a shared_ptr you are good:

void foo() {
  AMSMessage msg (...);
  std::shared_ptr<uint8_t> ptr(msg.data());
} // msg._data is freed here

However, it is counterintuitive and not RAII. Ideally, msg._data would be smart pointer from the beginning.

This is valid. We should at some point fix this. Not in this PR as it sounds orthogonal. Can you post an issue?

@koparasy koparasy self-requested a review March 21, 2025 16:58
@lpottier lpottier self-assigned this Mar 21, 2025
Signed-off-by: Loic Pottier <pottier1@llnl.gov>
Copy link
Member

@koparasy koparasy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@koparasy koparasy merged commit 470bdb6 into develop Mar 21, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants